Skip to content

Comments

[KYUUBI #5475][FOLLOWUP] Authz check permanent view's subquery should check view's correct privilege#5476

Closed
AngersZhuuuu wants to merge 5 commits intoapache:masterfrom
AngersZhuuuu:KYUUBI-5475
Closed

[KYUUBI #5475][FOLLOWUP] Authz check permanent view's subquery should check view's correct privilege#5476
AngersZhuuuu wants to merge 5 commits intoapache:masterfrom
AngersZhuuuu:KYUUBI-5475

Conversation

@AngersZhuuuu
Copy link
Contributor

@AngersZhuuuu AngersZhuuuu commented Oct 19, 2023 •
edited by yaooqinn
Loading

Why are the changes needed?

To fix #5475

In issue #5417 we fixed the problem that AUTHZ will still check scalar-subquery/in-subquery in permanent will.
But we just ignore the check, the subquery still will run, in this PR, we record the permanent view's visited column to check the permanent view's privilege to avoid extra execution effort.

For the test [KYUUBI #5417] should not check scalar-subquery in permanent view I print all the plan that pass to privilege builder as below
截屏2023-10-19 下午4 05 46

before this pr
截屏2023-10-19 下午4 15 29

This two graph shows this pr deny the execution of subquery when we don't have the veiw's privilege

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before make a pull request

Was this patch authored or co-authored using generative AI tooling?

@AngersZhuuuu
Copy link
Contributor Author

AngersZhuuuu commented Oct 19, 2023

ping @yaooqinn

@codecov-commenter
Copy link

codecov-commenter commented Oct 19, 2023 •
edited
Loading

Codecov Report

Merging #5476 (f7585a4) into master (03d6223) will not change coverage.
Report is 2 commits behind head on master.
The diff coverage is 0.00%.

❗ Current head f7585a4 differs from pull request most recent head e1f7920. Consider uploading reports for the commit e1f7920 to get more accurate results

@@          Coverage Diff           @@
##           master   #5476   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files         588     588           
  Lines       33480   33479    -1     
  Branches     4405    4402    -3     
======================================
+ Misses      33480   33479    -1
Files Coverage Δ
.../kyuubi/plugin/spark/authz/PrivilegesBuilder.scala 0.00% <0.00%> (ø)
.../plugin/spark/authz/util/PermanentViewMarker.scala 0.00% <0.00%> (ø)
...rk/authz/ranger/RuleApplyPermanentViewMarker.scala 0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cxzl25
Copy link
Contributor

cxzl25 commented Oct 19, 2023

Incomplete image link in PR description.

@AngersZhuuuu
Copy link
Contributor Author

AngersZhuuuu commented Oct 19, 2023

Incomplete image link in PR description.

Yea, changed

@AngersZhuuuu
Copy link
Contributor Author

AngersZhuuuu commented Oct 20, 2023

ping @yaooqinn @bowenliang123 could you table a look?

@bowenliang123 bowenliang123 changed the title [KYUUBI #5475] Authz check permanent view's subquery should check view's correct privilege [KYUUBI #5475][FOLLOWUP] Authz check permanent view's subquery should check view's correct privilege Oct 21, 2023
@AngersZhuuuu
Copy link
Contributor Author

AngersZhuuuu commented Oct 23, 2023

Any suggestion?

@yaooqinn
Copy link
Member

yaooqinn commented Oct 23, 2023

Can we add some tests for it?

@AngersZhuuuu
Copy link
Contributor Author

AngersZhuuuu commented Oct 23, 2023

Can we add some tests for it?

Existing ut already verified this? Since we skip check the subquery expression's privilege check

@yaooqinn
Copy link
Member

yaooqinn commented Oct 23, 2023 •
edited
Loading

The existing UT seems insufficient as it only has one col.

Let's update the test at least with create view v1 as select id, name, max(age) as max_age from xxxx.... to test

@AngersZhuuuu
Copy link
Contributor Author

AngersZhuuuu commented Oct 23, 2023

The existing UT seems insufficient as it only has one col.

Let's update the test at least with create view v1 as select id, name, max(age) as max_age from xxxx.... to test

How about current?

bowenliang123
bowenliang123 approved these changes Oct 23, 2023
View reviewed changes
Copy link
Contributor

@bowenliang123 bowenliang123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

yaooqinn
yaooqinn reviewed Oct 23, 2023
View reviewed changes
...rc/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyPermanentViewMarker.scala
}
PermanentViewMarker(resolvedSubquery, resolvedSubquery.desc)
PermanentViewMarker(
resolvedSubquery,
Copy link
Member

@yaooqinn yaooqinn Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it's not resolvedSubquery but a view?

@yaooqinn yaooqinn added this to the v1.9.0 milestone Oct 23, 2023
@yaooqinn yaooqinn closed this in 9e2fcc3 Oct 23, 2023
@yaooqinn
Copy link
Member

yaooqinn commented Oct 23, 2023

Thanks, merged to master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bowenliang123 bowenliang123 bowenliang123 approved these changes

@yaooqinn yaooqinn yaooqinn approved these changes

Assignees

@AngersZhuuuu AngersZhuuuu

Labels

module:authz module:extensions module:spark

Projects

None yet

Milestone

v1.9.0

Development

Successfully merging this pull request may close these issues.

[TASK][EASY] Authz check permanent view's subquery should check view's correct privilege

5 participants

@AngersZhuuuu @codecov-commenter @cxzl25 @yaooqinn @bowenliang123